Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Wizard Item Recall Spell #34411

Open
wants to merge 27 commits into
base: master
Choose a base branch
from
Open

Wizard Item Recall Spell #34411

wants to merge 27 commits into from
+375 −20

Conversation

ScarKy0
Copy link
Contributor

@ScarKy0 ScarKy0 commented Jan 13, 2025 •
edited
Loading

About the PR

Title.

Why / Balance

Missing feature for wizard.

Technical details

Added an ItemRecallSystem that handles the Item Recall spell.
Added RaiseOnAction to the BaseActionComponent to allow raising events on the action itself.

Media

2025-01-13.23-50-15.mp4

Requirements

Breaking changes

Changelog

not player facing (wizard when :( )

@github-actions github-actions bot added S: Untriaged Status: Indicates an item has not been triaged and doesn't have appropriate labels. size/M Denotes a PR that changes 100-999 lines. labels Jan 13, 2025
@SlimmSlamm
Copy link
Contributor

would this work if its in someone elses inventory/locker?

@ScarKy0
Copy link
Contributor Author

ScarKy0 commented Jan 13, 2025

would this work if its in someone elses inventory/locker?

If the item exists, no matter where, it WILL return to you

@SlimmSlamm
Copy link
Contributor

would this work if its in someone elses inventory/locker?

If the item exists, no matter where, it WILL return to you

thats awesome. hope to see some interactions with storage implanters or something. that would be hilarious.

@0x6273
Copy link
Contributor

0x6273 commented Jan 13, 2025

Isn't there a freeze on wizard and new spells?

@keronshb
Copy link
Contributor

Isn't there a freeze on wizard and new spells?

This has permission from me.

@ScarKy0 ScarKy0 marked this pull request as ready for review January 13, 2025 23:09
@ScarKy0 ScarKy0 changed the title [Draft] Wizard Item Recall Spell Wizard Item Recall Spell Jan 13, 2025
@github-actions github-actions bot added the S: Needs Review Status: Requires additional reviews before being fully accepted label Jan 13, 2025
@everydayeldergod
Copy link

should it do some damage if it uh... rips a storage implanter out of someone's chest? or is it more of a magical teleport?

slarticodefast
slarticodefast requested changes Jan 14, 2025
View reviewed changes
Copy link
Member

@slarticodefast slarticodefast left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did not test it in game yet.
Design-wise, did you ask Keron if it should be one or two actions? One for marking, one for recalling. That way you change the mark to another item if you wanted to.

Content.Client/ItemRecall/ItemRecallSystem.cs Show resolved Hide resolved
Content.Server/ItemRecall/ItemRecallSystem.cs Show resolved Hide resolved
Content.Shared/Projectiles/SharedProjectileSystem.cs Outdated Show resolved Hide resolved
Content.Shared/Actions/SharedActionsSystem.cs Outdated Show resolved Hide resolved
Content.Shared/ItemRecall/ItemRecallEvents.cs Outdated Show resolved Hide resolved
Content.Shared/ItemRecall/SharedItemRecallSystem.cs Outdated Show resolved Hide resolved
Content.Shared/ItemRecall/SharedItemRecallSystem.cs Outdated Show resolved Hide resolved
Content.Shared/ItemRecall/SharedItemRecallSystem.cs Outdated Show resolved Hide resolved
Content.Shared/ItemRecall/SharedItemRecallSystem.cs Outdated Show resolved Hide resolved
Content.Shared/ItemRecall/SharedItemRecallSystem.cs Outdated Show resolved Hide resolved
@ScarKy0
Copy link
Contributor Author

ScarKy0 commented Jan 14, 2025

I did not test it in game yet.
Design-wise, did you ask Keron if it should be one or two actions? One for marking, one for recalling. That way you change the mark to another item if you wanted to.

Spoke with Keron, youre not supposed to be able to mark another item(mark is permanent unless item somehow is destroyed) and it should be one action.
Thanks for the review ill address it all later

@slarticodefast slarticodefast added S: Awaiting Changes Status: Changes are required before another review can happen and removed S: Needs Review Status: Requires additional reviews before being fully accepted labels Jan 14, 2025
@ScarKy0
Copy link
Contributor Author

ScarKy0 commented Jan 14, 2025

should it do some damage if it uh... rips a storage implanter out of someone's chest? or is it more of a magical teleport?

As cool as this sounds, magical teleport

@ScarKy0 ScarKy0 requested review from keronshb February 1, 2025 23:21
@github-actions github-actions bot added the S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted label Feb 2, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 2, 2025

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@ScarKy0
Copy link
Contributor Author

ScarKy0 commented Feb 2, 2025

Insanity

@github-actions github-actions bot removed the S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted label Feb 2, 2025
metalgearsloth
metalgearsloth reviewed Feb 3, 2025
View reviewed changes
Content.Shared/Actions/SharedActionsSystem.cs
Comment on lines 681 to +684

if (action.RaiseOnAction)
target = actionId;

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This still looks like a code smell, especially when there's 2 separate bools controlling the same state.

@metalgearsloth metalgearsloth added S: Awaiting Changes Status: Changes are required before another review can happen and removed S: Needs Review Status: Requires additional reviews before being fully accepted labels Feb 3, 2025
@ScarKy0 ScarKy0 added the S: Needs Engine PR Merged Status: Requires an existing Robust Toolbox PR to be merged first. label Feb 3, 2025
@github-actions github-actions bot added S: Needs Review Status: Requires additional reviews before being fully accepted S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted and removed S: Awaiting Changes Status: Changes are required before another review can happen labels Feb 3, 2025
@github-actions GitHub Actions
Copy link
Contributor

github-actions bot commented Feb 6, 2025

This pull request has conflicts, please resolve those before we can evaluate the pull request.

@github-actions github-actions bot removed the S: Merge Conflict Status: Needs to resolve merge conflicts before it can be accepted label Feb 6, 2025
@ScarKy0 ScarKy0 removed the S: Needs Engine PR Merged Status: Requires an existing Robust Toolbox PR to be merged first. label Feb 6, 2025
@ScarKy0
Copy link
Contributor Author

ScarKy0 commented Feb 7, 2025

The errors are not my fault (to my knowledge)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@slarticodefast slarticodefast Awaiting requested review from slarticodefast

@keronshb keronshb Awaiting requested review from keronshb

@metalgearsloth metalgearsloth Awaiting requested review from metalgearsloth

Requested changes must be addressed to merge this pull request.

Assignees
No one assigned
Labels
A: Combat Area: Combat features and changes, balancing, feel A: Roundflow/Antag Area: Roundflow - "What happens in the game", including antagonist roles and their capabilities Changes: Sprites Changes: Might require knowledge of spriting or visual design. D2: Medium Difficulty: A good amount of codebase knowledge required. P2: Raised Priority: Item has a raised priority, indicating it might get increased maintainer attention. S: Needs Review Status: Requires additional reviews before being fully accepted size/M Denotes a PR that changes 100-999 lines. T: New Feature Type: New feature or content, or extending existing content
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
7 participants
@ScarKy0 @SlimmSlamm @0x6273 @keronshb @everydayeldergod @metalgearsloth @slarticodefast